Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make virtualize_frameworks the default #483

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

ob
Copy link
Member

@ob ob commented May 19, 2022

Since this performs better and we keep telling people to enable it, maybe we should make it the default?

Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is now pretty robust and stabilized and ready for prime. Most of the big builds I've heard about using rules_ios had to enable it because the perf was bad otherwise. Atop from build perf, the IDE is improved with this from multiple angles!

Do you have a strong reason to rip it out: e.g. it's hard to maintain or broken for the other case? Otherwise, I'd suggest to add an opt in / add opt out flag --features apple.incompatible_disable_virtual_frameworks and give it ~6-12 months to bake. This will have the effect of migrating people and giving ones we break reasonable to fix their issues or doing something else it if they don't / can't want to use it like this.

@jerrymarino
Copy link
Contributor

We can also remove the dup'd over github action shards when it's gone!

@ob
Copy link
Member Author

ob commented May 21, 2022

Do you have a strong reason to rip it out: e.g. it's hard to maintain or broken for the other case? Otherwise, I'd suggest to add an opt in / add opt out flag --features apple.incompatible_disable_virtual_frameworks and give it ~6-12 months to bake. This will have the effect of migrating people and giving ones we break reasonable to fix their issues or doing something else it if they don't / can't want to use it like this.

Yeah, this is a better plan.

@luispadron
Copy link
Collaborator

There might be some tests to add / use cases to fix before we enable this as default. For example: #500 does not work with VFS enabled. Im not sure if this is a one-off issue or if we'll see more issues like this by flipping it on.

@luispadron
Copy link
Collaborator

#500 is now closed so making this the default / cleaning up where the flags are used seems like a reasonable change.

@jerrymarino
Copy link
Contributor

I'm still hoping to make it default but leave it in - unless it becomes a big maintenance problem for us or other reasons to really gut it! If you've got to another use case e.g. #500 we should try to triage and resolve them. Making it default also might encourage people with issues to fix it but atleast give the escape hatch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants